-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[sqlqueryreceiver] - add optional static_attributes
to metrics.
#11868
[sqlqueryreceiver] - add optional static_attributes
to metrics.
#11868
Conversation
Also it would be nice to have this PR broken into several ones since the fixes are unrelated to each other. And don't forget to add change log entries :) |
I can close this and split it into two. I agree it makes more sense to have two PRs one for the addition of static tags, and another for the timestamp addition. |
@dmitryax removed the timestamp fix and will move to a different PR. Leaving this open for addition of static tag feature. |
@Konig-Corey please update PR description |
and a changelog entry is still required |
We just recently introduced another way to handle changelog. Please follow the updated process https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/CONTRIBUTING.md#adding-a-changelog-entry |
@dmitryax Added new change log. |
@Konig-Corey please rebase |
rebased from upstream main branch. |
static_attributes
to metrics.
please take a look at CI failures |
@@ -44,6 +45,8 @@ receivers: | |||
- metric_name: movie.genres | |||
value_column: "count" | |||
attribute_columns: [ "genre" ] | |||
static_attributes: | |||
dbinstance: mydbinstance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if I want to set an attribute to true
? How will we add this if we need it in a backwards compatible way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im not sure what you are asking. It only takes string values. Any non string values would likely cause an issue or be converted to a string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The @mx-psi's point is that if we want to add attributes with different value types, we won't be able to do that without breaking existing configuration interface. So it's better to design it better in advance. Maybe something like:
static_attributes:
- key: dbinstance
string_value: mydbinstance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we still applying this configuration interface ^ ? @mx-psi what do you think bout it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think changing StaticAttributes
to map[string]interface{}
and then doing:
for k, v := range cfg.StaticAttributes {
switch v.(type) {
case string:
attrs.InsertString(k, v)
// ... other cases here, or even just a default that returns an error saying non-string is not supported
}
Should work, and we don't have to complicate the configuration schema. Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our config provider definitely allows doing that: https://go.dev/play/p/CsyC3KuTer5
The yes/no case is an interesting edge case. The YAML package we use documents this explicitly here, in this case it will be a string since the field we unmarshal into is not a bool (see playground above). I think that's an acceptable behavior for me (but I understand if you feel otherwise).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with this approach. @Konig-Corey can you please keep existing interface but make sure that only string values are accepted for now. Later we can add support for other types if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After giving it a second thought, I would say I'd rather prefer explicit type definition in yaml config rather than relying on implying type from the value. As a user, I would rather be explicit about what type of values I will get, instead of figuring out what's the parsing logic to make sure that attribute has a type that I want. I don't think users will be setting many static attributes, usually one or two per metric, having another line in the settings per attribute is not a big overhead IMO.
Specifying attributes in a list also makes the emitted metric with attributes in predictable order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess not everyone has the same familiarity with YAML's quirkiness so that's a good argument.
Another reason I did not mention before to favor the map approach is that we sort-of have that on core here. I don't feel very strongly in favor of either implementation, but we should probably be consistent on both places. I don't want to drag this on forever, so I am fine with trying the explicit type here and then deciding which one we like the most.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, let's keep the same approach in this processor. I believe we can keep existing config behavior, given that the component is in alpha state. We can add restriction to string only attribute later.
Looks like a rebase went wrong? |
Working to fix it. Sorry about that. |
4138f90
to
baa3bf1
Compare
Co-authored-by: Dmitrii Anoshin <[email protected]>
baa3bf1
to
3e0e7e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Sorry for the delay
@mx-psi PTAL |
Description:
Adding a feature:
Testing:
Documentation:
FYI @pmcollins